Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bids_import] Added validSamples to index.json + Self-descriptive renames #1030

Merged

Conversation

jeffersoncasimir
Copy link
Contributor

This PR resolves the data shift issue mentioned in aces/Loris#8892 by adding the number of valid samples in the last chunk to the index.json.

It has a Loris PR counterpart: aces/Loris#8999 to use this value and address the fact that some calculations were being made by multiplying the number of chunks by the number of values per chunk, assuming they were all full.

@jeffersoncasimir jeffersoncasimir added A-BIDS Area: BIDS. Issues and pull requests related to BIDS and the BIDS import pipeline A-EEG Area: EEG. Issues and pull requests related to EEG / iEEG processing labels Dec 6, 2023
driusan pushed a commit to aces/Loris that referenced this pull request Jan 23, 2024
This resolves the data shift issue by adding the number of valid samples in the last chunk to the index.json, and using it for the calculations which previously assumed all chunks were filled.

Closes #8992. Has a Loris-MRI counterpart: aces/Loris-MRI#1030

Loosely dependant on #8998 (file modifications in common and done at a later time).
@cmadjar
Copy link
Collaborator

cmadjar commented Feb 16, 2024

@jeffersoncasimir who would best the best person to review this PR?

@jeffersoncasimir
Copy link
Contributor Author

@cmadjar I think all the developers who were given a test-25 clone VM (https://redmine.cbrain.mcgill.ca/issues/20684) should be able to test this with relatively little set-up time.

The list, excluding myself: @laemtl, @nicolasbrossard, @CamilleBeau, @regisoc

@cmadjar
Copy link
Collaborator

cmadjar commented Feb 20, 2024

@laemtl @nicolasbrossard @CamilleBeau @regisoc could one of you review this PR? Thank you :-)

@laemtl laemtl self-requested a review February 20, 2024 15:35
@cmadjar cmadjar merged commit 587560a into aces:main Feb 21, 2024
1 check passed
jeffersoncasimir added a commit to jeffersoncasimir/Loris that referenced this pull request Feb 29, 2024
This resolves the data shift issue by adding the number of valid samples in the last chunk to the index.json, and using it for the calculations which previously assumed all chunks were filled.

Closes aces#8992. Has a Loris-MRI counterpart: aces/Loris-MRI#1030

Loosely dependant on aces#8998 (file modifications in common and done at a later time).
@cmadjar cmadjar added this to the 26.0.0 milestone Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-BIDS Area: BIDS. Issues and pull requests related to BIDS and the BIDS import pipeline A-EEG Area: EEG. Issues and pull requests related to EEG / iEEG processing Priority high - my LORIS friend got merged!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants